Skip to content

fix: correctly bump justhtml; add config update to handle breaking changes#17279

Closed
stevejalim wants to merge 2 commits into
mainfrom
hotfix-integration-tests--dependabot-miss
Closed

fix: correctly bump justhtml; add config update to handle breaking changes#17279
stevejalim wants to merge 2 commits into
mainfrom
hotfix-integration-tests--dependabot-miss

Conversation

@stevejalim

@stevejalim stevejalim commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes integration test fails on VPN /thanks/ page auto-download, which broke silently when dependabot bumped justhtml==1.18.0 to 1.22.0 in prod.txt only (f054c29) - eg https://github.com/mozilla/bedrock/actions/runs/28828525342

Root cause

justhtml 1.19.0 hardened UrlPolicy: a UrlRule with no explicit handling now inherits UrlPolicy.default_handling per-rule. Bedrock's policy uses default_handling="strip" with allow-rules for a[href], img[src], img[srcset], so href/src` attributes on allowlisted tags started getting silently stripped.

On the VPN /thanks/ pages the download anchor lost its href, thanks.js read an empty value, and Playwright's waitForEvent('download') timed out.

XSS protection is unchanged .

Unit tests didn't catch it because requirements/dev.txt still pinned justhtml==1.18.0, so pytest ran against the old (pre-hardening) semantics.

Test plan

  • Unit tests pass locally, in particular bedrock/base/tests/test_sanitization.py (37 cases, now running under 1.22 too).
  • Integration tests pass on run-integration-tests - look for run-integration-tests @ 4fa4c79 on https://github.com/mozilla/bedrock/actions/workflows/integration_tests.yml
  • Locally, reload legal docs pages (./manage.py update_legal_docs --force) and eyeball for regressions (per the prod.in comment about manual QA on justhtml bumps).

@stevejalim stevejalim requested a review from a team as a code owner July 7, 2026 16:09
@stevejalim stevejalim requested a review from Copilot July 7, 2026 16:09
Copilot AI reviewed Jul 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a silent break caused by a justhtml upgrade in production by (1) aligning the pinned justhtml version across environments and (2) updating Bedrock’s URL sanitization policy to match justhtml >= 1.19’s per-rule default-handling behavior, restoring expected href/src preservation on allowlisted tags.

Changes:

  • Bump justhtml to 1.22.0 in production inputs and compiled requirement locks, and align dev to run tests against the same semantics.
  • Update bedrock/base/sanitization.py to explicitly set UrlRule(..., handling="allow") so allowlisted a[href] / img[src|srcset] keep scheme-validated URLs under the stricter UrlPolicy defaults.
  • Refresh compiled requirement locks (minor patch-level updates to other dependencies as part of the recompile).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
requirements/prod.txt Refresh compiled prod lockfile (includes justhtml==1.22.0 and related patch bumps).
requirements/prod.in Pin justhtml==1.22.0 in the prod input set.
requirements/docs.txt Refresh compiled docs lockfile (notably click patch bump).
requirements/dev.txt Refresh compiled dev lockfile to align with updated dependency set (includes justhtml==1.22.0).
bedrock/base/sanitization.py Explicitly set UrlRule handling to preserve allowlisted URL attributes under newer justhtml behavior.

@codecov

codecov Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.80%. Comparing base (af49d1b) to head (4bbcf5b).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #17279   +/-   ##
=======================================
  Coverage   82.80%   82.80%           
=======================================
  Files         178      178           
  Lines        9408     9408           
=======================================
  Hits         7790     7790           
  Misses       1618     1618

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stephaniehobson

stephaniehobson commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Looks like the integration tests failed :(

@stephaniehobson
Update VPN HTML
4bbcf5b 
- some HTML render "errors" were "fixed" and buttons were displaying wrong
  The HTML was wrong, but not having those classes rendered was what the
  tests and CSS were expecting.
@stephaniehobson stephaniehobson requested a review from a team as a code owner July 7, 2026 18:48
@stephaniehobson

stephaniehobson commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Added a fix for the VPN pages (broken HTML was fixed and that... broke something AFAICT).

stevejalim
stevejalim commented Jul 7, 2026
View reviewed changes
Comment thread package-lock.json
"version": "2.3.2",
"resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz",
"integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==",
"dev": true,

@stevejalim stevejalim Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephaniehobson is this addition intentional? I'll leave this PR for you to tweak and/or merge as you need

@stephaniehobson stephaniehobson Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was doing my usual "push so I can see the diff in GitHub" workflow. Which works great when I'm not collaborating 🙈

But, no, it's not intentional and I see it now. 👍

@stephaniehobson stephaniehobson Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how it got there though, shouldn't that reflect the config in the requirements files? I didn't edit them.

stephaniehobson
stephaniehobson reviewed Jul 7, 2026
View reviewed changes
Comment thread bedrock/products/tests/test_views.py
assert template == "products/monitor/waitlist/scan.html"


@override_settings(DEV=False, VPN_ENDPOINT="https://vpn.mozilla.org/")

@stephaniehobson stephaniehobson Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevejalim What do you think of this added test? Good addition or too specific?

@stephaniehobson

stephaniehobson commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Tests are still failing 😫

@stephaniehobson

stephaniehobson commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

I think the right thing to do is revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephaniehobson stephaniehobson stephaniehobson approved these changes
Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stevejalim @stephaniehobson